Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CI with stricter meson-python #232

Merged
merged 3 commits into from
Apr 25, 2023
Merged

Conversation

h-vetinari
Copy link
Member

Unexpected breakage in #200 (after #231 passed a few days ago). Investigating

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@h-vetinari h-vetinari changed the title retrigger CI TEST: investigate broken CI Apr 23, 2023
@h-vetinari
Copy link
Member Author

@rgommers @eli-schwartz @isuruf
Since we merged #231 a few days ago, something broke about the cross-compilation here. I'm not sure which bit of the infra is responsible, meson didn't even have a new build during that time.

The build runs fine (at least so it appears; no new or scary warnings from what I can tell), but then the very first import (AFAICT) during testing stumbles:

import: 'scipy'
Traceback (most recent call last):
  File "$TEST_ENV/lib/python3.11/site-packages/scipy/__init__.py", line 154, in <module>
    from scipy._lib._ccallback import LowLevelCallable
  File "$TEST_ENV/lib/python3.11/site-packages/scipy/_lib/_ccallback.py", line 1, in <module>
    from . import _ccallback_c
ImportError: cannot import name '_ccallback_c' from 'scipy._lib' ($TEST_ENV/lib/python3.11/site-packages/scipy/_lib/__init__.py)

@rgommers
Copy link
Contributor

We merged meson-python 0.13.0 yesterday, so that's probably it: conda-forge/meson-python-feedstock#15. The patch for cross-compilation was removed because it was included in that release already. So it's probably not that - but something unrelated may have broken.

@rgommers
Copy link
Contributor

From gh-231:

Build type: cross build
...
Build machine cpu family: aarch64
Build machine cpu: aarch64
Host machine cpu family: aarch64
Host machine cpu: aarch64
Target machine cpu family: aarch64
Target machine cpu: aarch64
...
Generating 'scipy/_lib/_ccallback_c.cpython-38-aarch64-linux-gnu.so.p/_ccallback_c.c'

This PR:

Build type: cross build
...
Build machine cpu family: x86_64
Build machine cpu: x86_64
Host machine cpu family: aarch64
Host machine cpu: aarch64
Target machine cpu family: aarch64
Target machine cpu: aarch64
...
Generating 'scipy/_lib/_ccallback_c.cpython-39-x86_64-linux-gnu.so.p/_ccallback_c.c'.

The reason is that reconfiguring an already-configured directory changed. In gh-231:

* Building wheel...
+ meson setup --prefix=$PREFIX $SRC_DIR $SRC_DIR/builddir --native-file=$SRC_DIR/.mesonpy-native-file.ini -Ddebug=false -Doptimization=2
Directory already configured.

In this PR:

* Building wheel...
+ meson setup --reconfigure $SRC_DIR $SRC_DIR/builddir -Dbuildtype=release -Db_ndebug=if-release -Db_vscrt=md --native-file=$SRC_DIR/builddir/meson-python-native-file.ini

IIRC that change was on purpose, and the "run meson setup builddir followed by pip install ..." was never guaranteed to work (but it did before). Let me refresh my memory though.

@rgommers
Copy link
Contributor

This is the current way of doing things in build.sh:

# need to run meson first for cross-compilation case
$PYTHON $(which meson) setup ${MESON_ARGS} \
    -Dblas=blas \
    -Dlapack=lapack \
    -Duse-g77-abi=true \
    builddir || (cat builddir/meson-logs/meson-log.txt && exit 1)

# -wnx flags mean: --wheel --no-isolation --skip-dependency-check
$PYTHON -m build -w -n -x -Cbuilddir=builddir

I think we need to change it to:

# -wnx flags mean: --wheel --no-isolation --skip-dependency-check
$PYTHON -m build -w -n -x -Csetup-args=-Dblas=blas -Csetup-args=-Dlapack=lapack -Csetup-args=-Duse-g77-abi=true

and then append the MESON_ARGS content. Right now that contains:

--buildtype release --prefix=xxx -Dlibdir=lib --cross-file xxx/meson_cross_file.txt

That's all fine, but meson-python wants it in the form -Csetup-args=<arg1> -Csetup-args=<arg2>. So what's needed is the bash foo to achieve that.

@rgommers
Copy link
Contributor

@h-vetinari, I'm guessing you know so I'll ask: is MESON_ARGS actually a space-separated string?

@h-vetinari
Copy link
Member Author

@h-vetinari, I'm guessing you know so I'll ask: is MESON_ARGS actually a space-separated string?

Yes it is. It's constructed here for osx, and here for linux.

That's all fine, but meson-python wants it in the form -Csetup-args= -Csetup-args=. So what's needed is the bash foo to achieve that.

This is going to be a general issue for all meson-python feedstocks, right? Perhaps it's a good idea to have a $MESON_PYTHON_ARGS that does this construction in the compiler activation? Then we'd avoid the bash-gymnastics in all the feedstocks that are moving to meson (numpy, scipy, pandas, etc.).

@eli-schwartz
Copy link

bash foo

The setup script for macOS uses bash, but the one for Linux indicates it tries to be compatible with POSIX sh. So no bash goodies.

(Like arrays!!! Arrays are really, really, really nice.)

@rgommers
Copy link
Contributor

Perhaps it's a good idea to have a $MESON_PYTHON_ARGS that does this construction in the compiler activation? Then we'd avoid the bash-gymnastics in all the feedstocks that are moving to meson (numpy, scipy, pandas, etc.).

That sounds quite nice, +1 from me.

@isuruf
Copy link
Member

isuruf commented Apr 23, 2023

I'm not a fan of adding a new variable to a big package like the compilers just for 2 feedstocks as meson-python is pretty niche right now. You can add the one liner "-Csetup-args=${MESON_ARGS// / -Csetup-args=}" here

@rgommers
Copy link
Contributor

Looks like it's 7 feedstocks right now: https://github.com/search?q=org%3Aconda-forge%20meson-python&type=code. And that will grow: PyWavelets completed the switchover, pandas is very close, numpy will follow too before Python 3.12. More smaller packages too I'd expect.

That said, if it's preferred to carry this one-liner (thanks for that providing it!) in the relevant feedstocks, that should work perfectly fine too.

Co-Authored-By: Isuru Fernando <isuruf@gmail.com>
Co-Authored-By: Ralf Gommers <ralf.gommers@gmail.com>
@h-vetinari
Copy link
Member Author

OK, it seems the scipy setup is setting -Dbuildtype=release somewhere, and now we're passing through --buildtype release, which causes:

+ meson setup $SRC_DIR $SRC_DIR/builddir -Dbuildtype=release -Db_ndebug=if-release -Db_vscrt=md -Dblas=blas -Dlapack=lapack -Duse-g77-abi=true --buildtype release --prefix=$PREFIX -Dlibdir=lib --native-file=$SRC_DIR/builddir/meson-python-native-file.ini

ERROR: Got argument buildtype as both -Dbuildtype and --buildtype. Pick one.

Guess I'll strip out that bit from $MESON_ARGS 😑

@rgommers
Copy link
Contributor

It's meson-python doing that. Yes, should be filtered out from MESON_ARGS.

@h-vetinari h-vetinari changed the title TEST: investigate broken CI Fix CI with stricter meson-python Apr 24, 2023
@h-vetinari h-vetinari marked this pull request as ready for review April 24, 2023 01:07
@h-vetinari
Copy link
Member Author

OK, something is still off. Despite correctly setting up the arguments now

Cython compiler for the build machine: cython (cython 0.29.34)
Build machine cpu family: x86_64
Build machine cpu: x86_64
Host machine cpu family: aarch64
Host machine cpu: aarch64
Target machine cpu family: aarch64
Target machine cpu: aarch64

meson starts building x86 libs on aarch/ppc

[10/1628] Compiling C object scipy/_lib/_test_ccallback.cpython-38-x86_64-linux-gnu.so.p/src__test_ccallback.c.o
[11/1628] Linking target scipy/_lib/_test_ccallback.cpython-38-x86_64-linux-gnu.so
[12/1628] Compiling C object scipy/_lib/_fpumode.cpython-38-x86_64-linux-gnu.so.p/_fpumode.c.o
[13/1628] Linking target scipy/_lib/_fpumode.cpython-38-x86_64-linux-gnu.so

The one thing that I see in the invocation

+ $PREFIX/bin/python -m build -w -n -x -Cbuilddir=builddir -Csetup-args=-Dblas=blas -Csetup-args=-Dlapack=lapack -Csetup-args=-Duse-g77-abi=true -Csetup-args=--prefix=$PREFIX -Csetup-args=-Dlibdir=lib -Csetup-args=--cross-file -Csetup-args=$BUILD_PREFIX/meson_cross_file.txt
* Building wheel...
+ meson setup $SRC_DIR $SRC_DIR/builddir -Dbuildtype=release -Db_ndebug=if-release -Db_vscrt=md -Dblas=blas -Dlapack=lapack -Duse-g77-abi=true --prefix=$PREFIX -Dlibdir=lib --cross-file $BUILD_PREFIX/meson_cross_file.txt --native-file=$SRC_DIR/builddir/meson-python-native-file.ini
The Meson build system
[...]

is the addition of --native-file=$SRC_DIR/builddir/meson-python-native-file.ini. Is that something done by meson-python? It sounds ("native") like it may override the cross-compilation settings?

@h-vetinari
Copy link
Member Author

is the addition of --native-file=$SRC_DIR/builddir/meson-python-native-file.ini. Is that something done by meson-python? It sounds ("native") like it may override the cross-compilation settings?

It looks like this might be related to mesonbuild/meson-python@c8dbaee, which landed in 0.13.0, or we simply didn't pick it up due to the fact that previously it only got appended for the second call to configure (and presumably ignored).

From the docs about the options, there also doesn't seem to be a way to switch of this native file, only add more to it:

The options that meson-python specifies by default are:

meson-python uses a native file to point Meson at the
python interpreter that the build must target. This is the
Python interpreter that is used to run the Python build
front-end. Meson would otherwise look for the first Python
interpreter on the $PATH, which may not be the same.

Additional --native-file options can be passed to meson setup
if further adjustments to the native environment need to be
made. Meson will merge the contents of all machine files. To ensure
everything works as expected, the meson-python native file is
last in the command line, overriding the python binary path
that may have been specified in user supplied native files.

I'm not sure if this is due to the not yet finished support for cross-compilation, but AFAICT, this automatic addition of --native-file is harmful for cross-compilation (we want to find the first Python on the $PATH that's been carefully set up), and we would need a way to turn it off.

@rgommers
Copy link
Contributor

Answered in mesonbuild/meson-python#321 (comment). With that fix, the logging should change from this:

Program python3 found: YES ($BUILD_PREFIX/bin/python3.10)

to showing $HOST_PREFIX I believe.

@h-vetinari
Copy link
Member Author

Answered in mesonbuild/meson-python#321 (comment). With that fix

Third time's the charm it seems - thanks a lot for the solution (I almost can't believe I got e1a936c correct first try 😅).

From my POV this should be ready to merge.

@rgommers
Copy link
Contributor

Did you see my comment on the native file part in that commit?

@h-vetinari
Copy link
Member Author

Did you see my comment on the native file part in that commit?

No sorry, I hadn't seen that in the UI here. Sorry if I got the instructions wrong, my impression was we need to pass a user-provided native-file to override the default one?

Are you saying that it should be enough to write the python host binary into the cross-file?

@rgommers
Copy link
Contributor

Are you saying that it should be enough to write the python host binary into the cross-file?

Yes indeed.

Co-Authored-By: Ralf Gommers <ralf.gommers@gmail.com>

# meson-python already sets up a -Dbuildtype=release argument to meson, so
# we need to strip --buildtype out of MESON_ARGS or fail due to redundancy
MESON_ARGS_REDUCED="$(echo $MESON_ARGS | sed 's/--buildtype release //g')"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h-vetinari do you think we could change MESON_ARGS to use -Dbuildtype instead? It is always documented like that (see https://mesonbuild.com/Builtin-options.html#details-for-buildtype) and is used a lot more than --buildtype if you look at the Meson issue tracker (in fact, it's hard to find --buildtype usage at all).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using -Dbuildtype; the line you commented on is stripping out the alternative formulation --buildtype.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But admittedly, that's the one coming from meson-python. We could change the spelling on the activation feedstock(s), that'd be fine by me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that spelling change is what I meant. That will avoid having to use this hack in all feedstocks that use meson-python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants